Prevent 0.0.0.0 day RCE vulnerability #1096
Conversation
dosubot
bot
added
size:L
|
This is one remediation strategy, but I actually don't think we need to take action here because browsers now should already handle the 0.0.0.0 day vulnerability. |
WalkthroughThis change implements a centralized CORS handling system across the server. A new Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/routes/mcp/servers.ts (1)
110-113: Use centralized logger instead ofconsole.debug.Per coding guidelines, server code should use the centralized logger utility rather than
console.*directly.- console.debug( - `Failed to disconnect MCP server ${serverId} during removal`, - error, - ); + logger.debug( + `Failed to disconnect MCP server ${serverId} during removal`, + error, + );
🧹 Nitpick comments (4)
server/utils/cors.ts (1)
3-11: ExportCorsOptionstype for external consumers.The
CorsOptionstype is used by callers ofbuildCorsHeadersbut isn't exported. Consider exporting it so consumers can type their options objects explicitly.-type CorsOptions = { +export type CorsOptions = { allowCredentials?: boolean; allowHeaders?: string; allowMethods?: string; exposeHeaders?: string; maxAge?: string; requestPrivateNetwork?: boolean; allowPrivateNetwork?: boolean; };server/routes/mcp/elicitation.ts (1)
52-56: Consider blocking SSE streams for disallowed origins.Currently, if
getAllowedOriginreturnsnull, the response proceeds withoutAccess-Control-Allow-Origin. While browsers will block cross-origin access to the response, the SSE connection itself is still established and data flows server-side.For defense-in-depth, you may wish to explicitly reject disallowed origins early:
const originHeader = c.req.header("origin"); const { headers: corsHeaders } = buildCorsHeaders(originHeader, { allowCredentials: true, }); + if (originHeader && !corsHeaders["Access-Control-Allow-Origin"]) { + return c.json({ error: "Origin not allowed" }, 403, { Vary: "Origin" }); + }server/index.ts (1)
235-241: Consider the interaction between manual validation andcors()middleware.The manual middleware (Lines 154-174) blocks disallowed origins, while this
cors()middleware adds CORS headers. Both referenceCORS_ORIGINS, which is good for consistency. However, the manual validation now renders some of this middleware's origin checking redundant.This layered approach is safe, though consolidating into a single CORS strategy could reduce complexity in the future.
server/routes/mcp/http-adapters.ts (1)
80-86: Remove shadowedoriginHeaderdeclaration.The
originHeadervariable is already declared at line 50 and accessible here. This redeclaration shadows the outer variable unnecessarily.// Compute an absolute endpoint based on forwarded headers when present // so direct access (without the proxy) advertises a reachable URL. const xfProto = c.req.header("x-forwarded-proto"); const xfHost = c.req.header("x-forwarded-host"); const host = xfHost || c.req.header("host"); let proto = xfProto; if (!proto) { - const originHeader = c.req.header("origin"); if (originHeader && /^https:/i.test(originHeader)) proto = "https"; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
server/index.ts(3 hunks)server/routes/mcp/elicitation.ts(3 hunks)server/routes/mcp/http-adapters.ts(6 hunks)server/routes/mcp/servers.ts(7 hunks)server/utils/cors.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
server/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
server/**/*.{ts,tsx,js,jsx}: Do not useconsole.log,console.warn, orconsole.errordirectly in server code; use the centralized logger utility from@/utils/loggerinstead
Use centralized logger utility methods:logger.error(),logger.warn(),logger.info(), andlogger.debug()from@/utils/loggerfor server-side logging
Files:
server/routes/mcp/elicitation.tsserver/index.tsserver/utils/cors.tsserver/routes/mcp/servers.tsserver/routes/mcp/http-adapters.ts
🧬 Code graph analysis (4)
server/routes/mcp/elicitation.ts (1)
server/utils/cors.ts (1)
buildCorsHeaders(49-84)
server/utils/cors.ts (1)
server/config.ts (2)
CORS_ORIGINS(18-23)SERVER_HOSTNAME(11-12)
server/routes/mcp/servers.ts (2)
server/utils/logger.ts (2)
logger(11-53)error(15-23)server/utils/cors.ts (1)
buildCorsHeaders(49-84)
server/routes/mcp/http-adapters.ts (1)
server/utils/cors.ts (1)
buildCorsHeaders(49-84)
🔍 Remote MCP
Summary of Additional Context for PR #1096 Review
This PR addresses a critical security vulnerability by implementing strict origin and host validation for localhost-based APIs. Here's the relevant context:
Vulnerability Context
The 0.0.0.0 Day vulnerability allows attackers to use public domains with 0.0.0.0 and "no-cors" mode to attack services running on localhost and gain arbitrary code execution (RCE), using a single HTTP request. The vulnerability affects all major browsers and enables RCE attacks on macOS and Linux devices.
The fix pattern used by ChromeDriver checks the Origin or Host headers and only allows localhost origins (localhost, 127.0.0.1, etc.). This PR implements a similar defensive strategy.
CORS Security Best Practices Applied
The PR's implementation aligns with critical security recommendations:
-
Origin Validation: The null origin should never be added to allowed domains, as it's used when requests come from privacy-sensitive contexts like sandboxed iframes, which attackers can include in their websites.
-
Credentials + Origin Restriction: If a request includes a credential (most commonly a Cookie header) and the response includes an Access-Control-Allow-Origin: * header, the browser will block access to the response. If response has Access-Control-Allow-Credentials: true, then the wildcard operator cannot be used on any of the response headers like Access-Control-Allow-Origin.
-
Wildcard Restrictions: CORS headers should be properly defined in respect of trusted origins for private and public servers, and avoid using wildcards in internal networks.
Private Network Access (PNA) Considerations
The PR adds support for Private Network Access, which is increasingly important:
Private Network Access (PNA) is a security feature that restricts the ability of websites to send requests to servers on private networks, helping protect users and internal networks from potential attacks like Cross-Site Request Forgery (CSRF).
Browsers send a CORS preflight request ahead of any private network request for a subresource, which carries a new header Access-Control-Request-Private-Network: true, and the response must carry a corresponding header Access-Control-Allow-Private-Network: true.
Key Review Points
- Host Header Validation: The new
isAllowedHost()function prevents DNS rebinding attacks by validating the Host header - Origin-Based Access Control: The
getAllowedOrigin()andbuildCorsHeaders()functions ensure only explicitly allowed origins can interact with the endpoints - Early Request Filtering: Host/origin validation happens at the middleware level, blocking attacks before they reach business logic
- Preflight Handling: The new
handlePreflight()for OPTIONS requests implements proper CORS preflight responses with origin validation - Credentials Flag: The implementation correctly uses
allowCredentials: trueonly after validating origins (never with wildcards)
[web_search]
🔇 Additional comments (8)
server/utils/cors.ts (2)
49-84: Well-structured CORS header construction.The implementation correctly:
- Always includes
Vary: Originfor proper caching behavior- Only sets
Access-Control-Allow-Originfor validated origins- Never combines credentials with wildcard origins
- Requires explicit opt-in for Private Network Access
43-47: Clarify the security implications of allowing requests without Host headers.The function returns
truefor missing or unparseablehostHeader, which means requests without a Host header bypass the validation check entirely. While this accommodates edge cases (HTTP/1.0 clients, internal tools), it contradicts the middleware's stated security goal of preventing "0.0.0.0 day" attacks that coerce local services into serving cross-origin traffic. Consider whether this permissive default is necessary or if Host headers should be required for validation.server/routes/mcp/elicitation.ts (1)
90-99: SSE response headers properly applied.The
corsHeadersare correctly spread into the response, maintaining the existing SSE-specific headers while adding origin-aware CORS headers.server/routes/mcp/servers.ts (1)
206-211: CORS header construction for RPC stream looks correct.Consistent with the pattern established in
elicitation.ts. Same defense-in-depth consideration applies: consider explicitly blocking disallowed origins before establishing the SSE connection.server/index.ts (2)
151-174: Solid host and origin validation middleware.This effectively mitigates the 0.0.0.0 day vulnerability by:
- Checking
x-forwarded-hostbeforehostfor proxy compatibility- Blocking requests with disallowed Host headers early
- Rejecting requests with unauthorized Origins before they reach handlers
250-268: Preflight handler properly validates origin and supports PNA.The implementation correctly:
- Blocks preflights from unauthorized origins with 403
- Supports Private Network Access headers
- Appends to
Varyheader appropriatelyserver/routes/mcp/http-adapters.ts (2)
20-40: Well-implemented preflight handler with origin validation.The
handlePreflightfunction correctly rejects unauthorized origins with 403 and supports PNA headers. Reusing this for both/:serverIdand/:serverId/*routes is clean.
47-65: Comprehensive CORS headers across all response paths.The
corsHeadersare consistently applied to HEAD, GET, POST, and error responses. This ensures browsers receive proper CORS headers regardless of the request outcome.
matteo8p
force-pushed
the
prevent-rce-attacks
branch
from
2d0fe92 to
264f107
Compare
dosubot
bot
added
size:XXL
matteo8p
force-pushed
the
prevent-rce-attacks
branch
from
264f107 to
1966067
Compare
This PR proposes a fix to the 0.0.0.0 day vulnerability where localhost applications are vulnerable to remote code execution. We add a PNA pre-flight with verification to reject requests from unverified origins.
#1095
support).